Skip to content

Conversation

@walhalabi-code
Copy link

@walhalabi-code walhalabi-code commented Jan 4, 2026

Summary

Fixed an issue where the default ACL defined in Class Level Permissions was incorrectly applied during object updates, overwriting any existing custom ACL.

Problem

When classLevelPermissions.ACL is configured for a class, the default ACL was being applied on every save operation (both create and update). This caused existing objects to lose their custom ACL settings when updated without an explicit ACL in the request.

Solution

Added a !this.query condition check before applying the default ACL. This ensures the default ACL is only set during object creation (when this.query is null), not during updates. This behavior is consistent with how other default fields like createdAt are handled in RestWrite.

Summary by CodeRabbit

  • Bug Fixes

    • Default ACLs are now applied only on object creation; updates no longer receive automatic default ACLs, preventing unintended permission changes.
  • Tests

    • Added tests ensuring updates do not overwrite existing ACLs and that explicit ACL modifications on update are honored.

✏️ Tip: You can customize this high-level summary in your review settings.

@parse-github-assistant
Copy link

parse-github-assistant bot commented Jan 4, 2026

🚀 Thanks for opening this pull request!

@parseplatformorg
Copy link
Contributor

parseplatformorg commented Jan 4, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai
Copy link

coderabbitai bot commented Jan 4, 2026

📝 Walkthrough

Walkthrough

Default ACL assignment in src/RestWrite.js was restricted to CREATE operations (no query); default ACLs are no longer auto-applied on UPDATE. Two tests were added in spec/ParseACL.spec.js to verify ACL preservation on update and explicit ACL modifications are honored.

Changes

Cohort / File(s) Summary
ACL Logic Update
src/RestWrite.js
Modified condition that applies default ACLs so it runs only for creates (no query present); prevents automatic insertion/overwriting of ACLs on updates.
ACL Behavior Tests
spec/ParseACL.spec.js
Added tests: one verifies default ACL is not applied/overwritten on update; another verifies explicit ACL changes on update are preserved.
Manifest
package.json
Manifest present in diff; no exported/public API signature changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The description explains the problem, solution, and approach clearly, but does not follow the required template structure and is missing the Issue link (Closes field) and Tasks checklist. Follow the repository's PR description template: add an 'Issue' section with 'Closes: [issue number]' and include the 'Tasks' checklist with relevant items marked.
✅ Passed checks (2 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly summarizes the main fix: preventing default ACL from overwriting custom ACL on object updates. It directly reflects the primary change in the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/RestWrite.js (1)

373-390: Add test for default ACL behavior on UPDATE operations.

The existing test "support defaultACL in schema" verifies that default ACL is applied during CREATE. However, there's no test verifying that updates to objects with default ACLs do not reapply or overwrite the existing ACL. Add a test that:

  1. Creates an object with a class-level default ACL
  2. Updates the object
  3. Verifies the ACL from creation is preserved and not replaced by the default ACL

Also cover the edge case where a user explicitly modifies the ACL during an update.

🧹 Nitpick comments (1)
src/RestWrite.js (1)

373-390: Suggestion: Refine PR title to include scope.

Based on coding guidelines, consider updating the PR title to:

fix(rest): prevent default ACL from overwriting custom ACL on update

This format makes it clearer in the changelog what specific behavior was fixed and why it matters to developers.

Based on learnings, Angular commit convention suggests including a scope (e.g., rest, acl) to identify the subsystem affected.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 774cc54 and 35d142a.

📒 Files selected for processing (1)
  • src/RestWrite.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
🔇 Additional comments (1)
src/RestWrite.js (1)

373-390: LGTM! Correct fix that prevents ACL overwrite on updates.

The addition of !this.query && correctly restricts default ACL assignment to CREATE operations only, preventing custom ACLs from being overwritten during UPDATE operations. This change is consistent with how other default fields like createdAt and objectId are handled (lines 393-422).

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 4, 2026
@walhalabi-code walhalabi-code changed the title fix: Apply default ACL only on object creation, not update fix(rest): prevent default ACL from overwriting custom ACL on update Jan 4, 2026
@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix(rest): prevent default ACL from overwriting custom ACL on update fix(rest): Prevent default ACL from overwriting custom ACL on update Jan 4, 2026
@walhalabi-code walhalabi-code changed the title fix(rest): Prevent default ACL from overwriting custom ACL on update fix: prevent default ACL from overwriting custom ACL on update Jan 4, 2026
@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix: prevent default ACL from overwriting custom ACL on update fix: Prevent default ACL from overwriting custom ACL on update Jan 4, 2026
@walhalabi-code walhalabi-code changed the title fix: Prevent default ACL from overwriting custom ACL on update fix(rest): prevent default ACL from overwriting custom ACL on update Jan 4, 2026
@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix(rest): prevent default ACL from overwriting custom ACL on update fix(rest): Prevent default ACL from overwriting custom ACL on update Jan 4, 2026
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 4, 2026
@walhalabi-code walhalabi-code force-pushed the fix-default-acl-on-update branch from 9414f35 to 5c830dc Compare January 4, 2026 11:16
@walhalabi-code
Copy link
Author

any updates...

Thank you... <3

@mtrezza
Copy link
Member

mtrezza commented Jan 27, 2026

@coderabbitai review the claim of this bug; this would be a major bug that should have surfaced earlier; if this bug really exists, what commit introduced it?

@coderabbitai
Copy link

coderabbitai bot commented Jan 27, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@mtrezza I'll investigate the claimed bug and trace its origin in the git history.


🏁 Script executed:

#!/bin/bash

# First, let's look at the actual changes in the PR
echo "=== PR Changes in src/RestWrite.js ==="
git diff origin/alpha...HEAD -- src/RestWrite.js

echo -e "\n\n=== Full context of setRequiredFieldsIfNeeded method ==="
# Get the full method to understand the context
ast-grep --pattern $'setRequiredFieldsIfNeeded() {
  $$$
}'

echo -e "\n\n=== Search for when default ACL handling was introduced ==="
# Search git log for when defaultACL or default ACL logic was added to RestWrite
git log --all --oneline --grep="ACL" --grep="default" -- src/RestWrite.js | head -20

echo -e "\n\n=== Check commits that modified the setRequiredFieldsIfNeeded area ==="
# Look for changes in this specific method
git log -p --all -S "setRequiredFieldsIfNeeded" -- src/RestWrite.js | head -200

Length of output: 1352


🏁 Script executed:

#!/bin/bash

# Get details about the commit that introduced default ACL
echo "=== Details of commit 12b5d78 that added default ACL ==="
git show 12b5d78 --stat

echo -e "\n\n=== Full diff of the default ACL introduction ==="
git show 12b5d78 -- src/RestWrite.js

echo -e "\n\n=== Check the date and PR details ==="
git log --format=fuller 12b5d78 -1

echo -e "\n\n=== Look at the actual test file changes in the current PR ==="
git diff origin/alpha...HEAD -- spec/ParseACL.spec.js | head -100

Length of output: 4965


🏁 Script executed:

#!/bin/bash

# Look at the tests added in the original PR `#8701` to see if they tested updates
echo "=== Tests added in commit 12b5d78 (PR `#8701`) ==="
git show 12b5d78 -- spec/ParseACL.spec.js

echo -e "\n\n=== Check if there are any UPDATE-related tests in the original commit ==="
git show 12b5d78 -- spec/ParseACL.spec.js | grep -i "update" -A 5 -B 5

Length of output: 1752


@mtrezza The bug claim is legitimate. Here's my analysis:

Bug Verification

The bug is real. The original implementation applies the default ACL on both CREATE and UPDATE operations, when it should only apply on CREATE.

Evidence from code:

  • Line 373-389 in current src/RestWrite.js applies default ACL whenever: (1) schema has ACL config, (2) no ACL in request data, (3) ACL is not public read/write
  • Missing condition: !this.query (which indicates CREATE vs UPDATE)
  • The very next code block for default fields (line 392) correctly uses !this.query to only apply on CREATE

When Was It Introduced?

Commit: 12b5d781dc3f8c43c0c566dffa9308d02a7d8043
PR: #8701
Date: March 25, 2025 (~10 months ago)
Author: Daniel daniel-blyth@live.com.au
Title: "feat: Add default ACL"

The bug was introduced in the initial implementation of the default ACL feature.

Why Wasn't It Caught Earlier?

  1. Incomplete test coverage in PR #8701: The original PR added only one test called "support defaultACL in schema" which tested object creation, but never tested updates

  2. Relatively new feature: Only 10 months old, so limited production exposure

  3. Specific conditions required: Bug only manifests when:

    • Class has default ACL configured in CLP
    • Update operation performed without explicit ACL in request
    • User expects existing custom ACL to persist
  4. Silent corruption: Objects don't fail to save; their ACL just gets unexpectedly replaced, which might not be immediately noticed

The Fix

The current PR correctly adds !this.query && to match the pattern used for other default fields, ensuring default ACL applies only on CREATE operations.


🧠 Learnings used
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.

@mtrezza mtrezza changed the title fix(rest): Prevent default ACL from overwriting custom ACL on update fix: Default ACL overwrites custom ACL on Parse.Object update Jan 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants